Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SPI driver for samg family #680

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

mcbridejc
Copy link
Contributor

Currently based off feature/samg-usb, on the assumption those will land in develop before this.

@mcbridejc
Copy link
Contributor Author

It pains me to admit that I've failed to understand the GPIO connect system implemented here, and not for lack of trying.

As I recall, the method of leaving some pins unconnected in the STM32 drivers is to pass, e.g. GpioUnused::Sck as a connect template parameter. I've tried to support a similar thing here for the sake of consistency, but have failed. Instead, I've just made all of the pins optional, and the connect() method will connect whatever subset of pins are provided.

@mcbridejc mcbridejc marked this pull request as ready for review September 10, 2021 18:41
src/modm/platform/spi/sam/spi_master.cpp.in Outdated Show resolved Hide resolved
src/modm/platform/spi/sam/spi_master.hpp.in Outdated Show resolved Hide resolved
src/modm/platform/spi/sam/spi_master.hpp.in Outdated Show resolved Hide resolved
examples/samg55_xplained_pro/spi-loopback/main.cpp Outdated Show resolved Hide resolved
@salkinium
Copy link
Member

salkinium commented Sep 10, 2021

As I recall, the method of leaving some pins unconnected in the STM32 drivers is to pass, e.g. GpioUnused::Sck as a connect template parameter.

No, you just don't pass anything. The template arguments have a variable length for this purpose. The STM32 connector just returns GpioUnused instead of void when searching for the signal in the parameter pack, which has all the same APIs as the other Gpio types, just with empty implemtnations.

Instead, I've just made all of the pins optional, and the connect() method will connect whatever subset of pins are provided.

That's fine, you haven't failed at all. This is also a valid way to implement it.

I remember something about the Flexcom getting in the way of the GpioUnused API, so I guess that's why the SAM port uses a different way?

@mcbridejc
Copy link
Contributor Author

No, you just don't pass anything. The template arguments have a variable length for this purpose. The STM32 connector just returns GpioUnused instead of void when searching for the signal in the parameter pack, which has all the same APIs as the other Gpio types, just with empty implemtnations.

Interesting. I have been doing SPI::connect<SCK::Sck, MOSI::Mosi, GpioUnused::Miso>();, and I vaguely remember figuring that out after getting errors when providing just two pins...but now I've removed the GpioUnused arg and indeed it works! So I'm not sure how I ended up coming to believe that...

I remember something about the Flexcom getting in the way of the GpioUnused API, so I guess that's why the SAM port uses a different way?

You're probably right; though I certainly don't know. It all feels overly complex to me, and I definitely feel like I'm poking at something I only vaguely understand. However I don't want to spend days/weeks figuring it out, and this seems to work, so I'm prepared to push on :).

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Sep 10, 2021
@mcbridejc
Copy link
Contributor Author

Awesome, thanks!

Thank you for maintaining this! You have done all the hard parts :)

@salkinium salkinium merged commit f4d5d6c into modm-io:develop Sep 10, 2021
@salkinium salkinium added this to the 2021q3 milestone Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:hal Triggers the exhaustive HAL compile CI jobs feature 🚧
Development

Successfully merging this pull request may close these issues.

2 participants